Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: respect the silence as defined in user LSP settings #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eugenesvk
Copy link

LSP is planning to add a user setting allowing for an uniterrupted user experience on various less-than-critical errors.

However, lsp_utils still asks for download in case some local node-resolution failed. This PR checks whether the user has opted into no interruptions in their LSP config and auto-cancels the download and the download prompt with a custom console log message specifying which setting is responsible

LSP will notify via statusbar that the server failed, so no need for an extra status notification in this package

addresses sublimelsp/LSP#2545

do not show a blocking UI window, instead auto-skipping download with a custom console log message

LSP will notify via statusbar that the server failed, so no need for an extra status notification here
Comment on lines +97 to 104
lsp_cfg = sublime.load_settings('LSP.sublime-settings')
suppress_error_dialogs = cast(bool, lsp_cfg.get('suppress_error_dialogs') or False)
if suppress_error_dialogs:
log_lines.append(' * Download auto-skipped due to "suppress_error_dialogs" setting')
continue
if not sublime.ok_cancel_dialog(
NO_NODE_FOUND_MESSAGE.format(package_name=package_name), 'Download Node.js'):
log_lines.append(' * Download skipped')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not strictly an error dialog but a dialog asking user to install local node instance. Doesn't seem right to me to tie it to that LSP setting.

There is already a way to prevent this dialog from appearing by setting "nodejs_runtime": ["system"] setting in lsp_utils settings.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I did want to add non-error dialog setting for technical cases like this :), but that got rejected.

I'd still like to use the local system in general, just not be block-ui-notified about any issues, so the use case is exactly the same, and I can decide whether to just ignore these messages if it's a temp issue with system node, and I don't need a dupe, or install a local node if I don't plan to install a system one.

Although in this specific case there are no commands to manually install, right? So in addition to suppressing the dialog (which is desired) this prevents any local installation (which isn't desired and would be hard to debug)

Could we add a command to install node locally and notify that this command could be used in the message?

Copy link
Member

@rchl rchl Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to use the system node in general then why don't you just set the nodejs_runtime as I've suggested? You clearly don't want to install a "local" node instance or you would do it once and never care about this dialog anymore.

Your crusade against modal dialog might be going too far in this case. The modality of the dialog is quite useful in this case since it allows user to install node instance and everything works right out of the box then. Otherwise (with just a log message), the LSP would fail to initialize and then the user would have to at least re-open the file after installing local node manually.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I want to be able to use either, not just the system one. And I don't install the local one when it's a dupe, but in cases when I didn't have a system one that's exactly what I did - installed the local version via this plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants